-
-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: change navdrop links and sub-links style on hover #463
base: master
Are you sure you want to change the base?
feat: change navdrop links and sub-links style on hover #463
Conversation
✅ Deploy Preview for peaceful-ramanujan-288045 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome enhancement contribution @gaurav-2-0-0-2, but i think a nice addition will be to show which link is currently active. wyt?
Yes I was thinking the same thing. will add changes to this PR |
Hey @gaurav-2-0-0-2 The second hover effects for mobile screens would look better as way of indicating where the user is as said by Ace. But I dont see a use of them in mobile view, as after clicking the links the user would simply be redirected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the changes in package-lock.json
do i need to remove package-lock.json when making a PR |
Just dont have any changes in that file |
and the page re renders and the navdrop set to what it was, do we need to highlight the sub menus here or will it be an overkill ? |
I don't think it will be an overkill tbh. Wyt @AceTheCreator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! cc @ashmit-coder
hey is this going to be merged, it is showing some conflicts ? |
yes @gaurav-2-0-0-2 you have to resolve those conflicts on your local and then push the new code. Do try not to hurt the conflicting functionality and maintain your changes as well. If you need help you can ask in slack. The topic you want to look at is |
hey @ashmit-coder i resolved the conflicts, made a small change to make the functionality work, is it good to go now ? do suggest if any changes required. |
@gaurav-2-0-0-2 did you make sure not to remove the functionality that was giving conflict. tell me the name of the file that was causing the conflict. |
Yes I made sure to not remove those changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@gaurav-2-0-0-2 kindly resolve the conflict on this PR 🙏🏽 |
hey @AceTheCreator resolved conflicts I think you can merge now |
Description
Resolves #462
Demo